-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
install: add --copy-network and --network-dir options #212
Conversation
This is my first ever rust PR. It took me waaaay too long to get this code working 😜. Please be gentle in code review. marking as WIP for now as I test things out more |
f8cff4b
to
97f4560
Compare
.short("n") | ||
.long("copy-network-config") | ||
.value_name("path") | ||
.default_value("/etc/NetworkManager/system-connections/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default value will likely need to be different for RHCOS (for now). I'm thinking we can just carry that as a patch in the RPM in downstream. Does that sound OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also do want to copy that path for RHCOS too. So it's more like disabling ifcfg copying on FCOS. We could do something like look at the target OS install and conditionalize based on that...since we do want to allow people to use the upstream/Fedora coreos-installer
binary to install RHCOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or to rephrase, FCOS supports NM config files; RHCOS supports NM config files and ifcfg files. It's not "RHCOS only supports ifcfg files".
Perhaps simplest is just to make this a boolean value, and copy files from both paths if they exist and the option is specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also do want to copy that path for RHCOS too. So it's more like disabling ifcfg copying on FCOS. We could do something like look at the target OS install and conditionalize based on that...since we do want to allow people to use the upstream/Fedora
coreos-installer
binary to install RHCOS.
Using the FCOS installer to install RHCOS is probably a little risky long term, though for this case the user could still pass -n /etc/sysconfig/network-scripts/
and get it to work.
Or to rephrase, FCOS supports NM config files; RHCOS supports NM config files and ifcfg files. It's not "RHCOS only supports ifcfg files".
Perhaps simplest is just to make this a boolean value, and copy files from both paths if they exist and the option is specified.
Since the use case here is to propagate forward the files generated by nmtui
I think we should support the default format that is spit out by nmtui
on each platform and not overcomplicate things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the use case here is to propagate forward the files generated by nmtui I think we should support the default format that is spit out by nmtui on each platform and not overcomplicate things.
Hmm. I suppose you're right that NM will save in ifcfg files by default on RHCOS. I wonder how hard that'd be to change though...i.e. make ifcfg files a read source but not a write target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. make ifcfg files a read source but not a write target.
+1, that's what it does for nm keyfiles today IIUC. I'd have to dig into the plugin to see if it's possible. For now I'm going to stick with this and I'll come back and change it if I have enough time between now and this Friday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened this against NM upstream to see if there's any way to do what we want: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/415
@@ -233,6 +236,53 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
/// Copy networking config if asked to do so | |||
fn copy_network_config(mountpoint: &Path, net_config_src: &str) -> Result<()> { | |||
eprintln!("Copying networking configuration from {}", net_config_src); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this isn't an error, let's use println!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will look into this. Thanks for the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, all of our existing status logging goes to stderr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'll leave this as is in this PR, but it's probably worth us having a followup where we convert our non-error status logging to non stderr, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Status reports aren't the primary output of the program, so it makes sense to me that they would go to stderr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If we ever add e.g. a JSON report describing what actions install
has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.
@lucab, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If we ever add e.g. a JSON report describing what actions
install
has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.
Sounds like a good use case for a --quiet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way maybe we should move this conversation into an issue since it no longer really reflects a change we are going to make in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow up: #214
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. If we ever add e.g. a JSON report describing what actions install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.
What you're talking about is one of the most long-standing things in how Unix pipelines work - the fact they're just connected by bytestreams and there's no common APIs or structure. A lot of successor projects try to address this, from Windows Powershell to nushell to many others. (In fact long ago I tried to write one myself...)
Practically speaking, I think it's better to do e.g. --status-report-json /path/to/status.json
. Or alternatively --output=json
and that also suppresses internal printing.
Another approach would be to convert the files into an Ignition config that gets merged with any other config provided. Something like having Ignition support a |
We discussed this a bit before I think: #205 (comment). We need the networking to be brought up in the initramfs. |
The systemd unit/script that will pick up these networking config files placed by coreos-installer is in coreos/fedora-coreos-config#346 |
I'll get to the code review comments/suggestions in this PR first thing tomorrow. I'll be doing a good dose of testing as well. |
src/cmdline.rs
Outdated
.value_name("path") | ||
.default_value("/etc/NetworkManager/system-connections/") | ||
.takes_value(true) | ||
.help("Optionally copy the network config from <path> in the installer environment"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the 80-column limit on readable help text. Copy network config from <path>
almost fixes it, but the option description (--copy-network-config <path>
) is so wide that it shifts other options' help text to the right.
Which intersects with another issue: I don't think the single option is going to work. coreos-installer install -n /dev/loop0
and coreos-installer install --copy-network-config /dev/loop0
both misinterpret /dev/loop0
as the path to the network configs. Without a more discerning parser, I think we should split this into two options: a boolean, and an option to override the default path.
Dropping <path>
from the option description would fix the help text length without needing to rename the option, though it then moves the problem to choosing the name of the second option.
I know the character-counting is painful but we don't really get usable help text otherwise. We could enable the wrap_help
clap feature but it's... only slightly less bad:
Install Fedora CoreOS or RHEL CoreOS
USAGE:
coreos-installer install [OPTIONS] <device>
OPTIONS:
-s, --stream <name> Fedora CoreOS stream [default: stable]
-u, --image-url <URL> Manually specify the image URL
-f, --image-file <path> Manually specify a local image file
-i, --ignition-file <path> Embed an Ignition config from a file
-p, --platform <name> Override the Ignition platform ID
--firstboot-args <args>
Additional kernel args for the first boot
-n, --copy-network-config <path>
Optionally copy the network config from <path> in the installer
environment [default: /etc/NetworkManager/system-connections/]
--insecure Skip signature verification
--stream-base-url <URL>
Base URL for Fedora CoreOS stream metadata
--architecture <name>
Target CPU architecture [default: x86_64]
--preserve-on-error Don't clear partition table on error
-h, --help Prints help information
ARGS:
<device> Destination device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the 80-column limit on readable help text.
Copy network config from <path>
almost fixes it, but the option description (--copy-network-config <path>
) is so wide that it shifts other options' help text to the right.Which intersects with another issue: I don't think the single option is going to work.
coreos-installer install -n /dev/loop0
andcoreos-installer install --copy-network-config /dev/loop0
both misinterpret/dev/loop0
as the path to the network configs. Without a more discerning parser, I think we should split this into two options: a boolean, and an option to override the default path.
I'll work on adding the second option.
Dropping
<path>
from the option description would fix the help text length without needing to rename the option, though it then moves the problem to choosing the name of the second option.
I'll try to get the text to under 80 characters.
I know the character-counting is painful but we don't really get usable help text otherwise. We could enable the
wrap_help
clap feature but it's... only slightly less bad:
On a console readability within 80 characters in important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closest I could get was changing the long form of -n
to --copy-network
, adding a --network-dir
option and using .next_line_help(true)
. Here's what we end up with, which is less than 80 characters and I think a good compromise:
Install Fedora CoreOS or RHEL CoreOS
USAGE:
coreos-installer install [OPTIONS] <device>
OPTIONS:
-s, --stream <name> Fedora CoreOS stream [default: stable]
-u, --image-url <URL> Manually specify the image URL
-f, --image-file <path> Manually specify a local image file
-i, --ignition-file <path> Embed an Ignition config from a file
-p, --platform <name> Override the Ignition platform ID
--firstboot-args <args> Additional kernel args for the first boot
-n, --copy-network Copy network config from install environment
--network-dir <path>
For use with -n. [default: /etc/NetworkManager/system-connections/]
--insecure Skip signature verification
--stream-base-url <URL> Base URL for Fedora CoreOS stream metadata
--architecture <name> Target CPU architecture [default: x86_64]
--preserve-on-error Don't clear partition table on error
-h, --help Prints help information
ARGS:
<device> Destination device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid the second line with e.g. Override default network config path
and hide_default_value(true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think showing the user the default is important.
97f4560
to
29e22ef
Compare
This will allow us to copy in networking configuration from a specified location into a predetermined directory inside of `/boot/` of the installed system (which follows the path of other postprocess steps). Having this mechanism will allow for a user to interactively configure networking in the Live ISO environment via `nmtui` and then have those settings propagate forward into the installed system. After the files are copied into the directory within `/boot/` a systemd unit from the initramfs will pick them up on firstboot and propagate them into the appropriate locations. Fixes: coreos#205
29e22ef
to
df06c7b
Compare
based on #212 (comment) I changed this to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely happy with the help text, but we can always change it later. The substance LGTM!
This will allow us to copy in networking configuration from a specified
location into a predetermined directory inside of
/boot/
of theinstalled system (which follows the path of other postprocess steps).
Having this mechanism will allow for a user to interactively configure
networking in the Live ISO environment via
nmtui
and then have thosesettings propagate forward into the installed system.
After the files are copied into the directory within
/boot/
a systemdunit from the initramfs will pick them up on firstboot and propagate
them into the appropriate locations.
Fixes: #205